test: Shared test infrastructure — MockHttpMessageHandler, fixtures & helpers (#51)#53
Merged
Merged
Conversation
- Add PeachPayments = 15 to PaymentGatewayType enum - Add PeachPaymentsConfig (EntityId, AccessToken, IsSandbox) - Implement PeachPaymentsGateway: checkout initiation, payment verification, refund - Register gateway in IServiceCollectionExtensions and PaymentGatewayFactory - Add ZAR/KES/NGN/BWP routing to PeachPayments in PaymentService - Add PEACH_ prefix detection for gateway resolution
… helpers (#51) - Add MockHttpMessageHandler: queue-based fake HTTP handler with assertion helpers (AssertRequestCount, AssertLastRequestPath, AssertLastMethod). Supports multi-step gateways via sequential queuing. - Add PaymentRequestFactory: builds valid PaymentRequest and RefundRequest with sensible defaults; supports configure lambda. - Add GatewayConfigFactory: one builder per gateway, sets only the minimum keys required to pass constructor validation. - Add IntegrationTestBase: abstract base that checks env vars and exposes SkipIfMissingEnvVars() for clean CI output. - Add paybridge.runsettings: supports dotnet test --filter Category=Unit and --filter Category=Integration with code coverage config. - Add TestInfrastructureTests: 20 unit tests covering all helpers. Closes #51
📝 WalkthroughWalkthroughAdds shared test utilities (mock HTTP handler, payment/gateway factories, integration test base, runsettings), unit tests for those utilities, CI workflows for unit and integration tests, and a full PeachPayments gateway implementation wired into DI and selection logic. ChangesTest Infrastructure and PeachPayments Gateway
Sequence Diagram(s): sequenceDiagram
participant Test as TestCode
participant Gateway as PeachPaymentsGateway
participant API as PeachPaymentsAPI
Test->>Gateway: CreatePaymentAsync(request)
Gateway->>API: POST /checkouts (Bearer auth)
API-->>Gateway: JSON response (result.code, id, description)
Gateway-->>Test: PaymentResponse (status, widget URL)
Estimated code review effort: Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs (1)
77-130: ⚡ Quick winAdd missing coverage for
AssertLastMethodandIntegrationTestBaseskip behavior.The suite validates most helper paths but skips two core behaviors from this infrastructure set: method assertion helper and env-var-driven skip logic. Adding these targeted tests would close the remaining acceptance-risk gaps.
Also applies to: 134-224
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs` around lines 77 - 130, Add two tests: one covering MockHttpMessageHandler.AssertLastMethod and one covering IntegrationTestBase's env-var-driven skip logic. For AssertLastMethod, mirror existing request-path tests: create a MockHttpMessageHandler, RespondWith(...), perform a request with HttpMethod.Post (and another with HttpMethod.Get), then call handler.AssertLastMethod(HttpMethod.Post) to assert it does not throw and a mismatched call to AssertLastMethod expects InvalidOperationException with a message containing the expected vs actual method. For IntegrationTestBase, add tests that set and unset the environment variable that IntegrationTestBase reads (e.g., RUN_INTEGRATION_TESTS or the actual key used in IntegrationTestBase) and assert the class/method that determines skipping (IntegrationTestBase.Skip/ShouldSkip/ShouldRunIntegrationTests) returns the expected boolean for both enabled and disabled values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs`:
- Around line 36-48: Modify the IntegrationTestBase constructor to load the
.env.test file before evaluating required env vars: call your dotenv loader
(e.g., DotNetEnv.Load(".env.test") or equivalent safe loader) at the start of
the constructor (before computing missing) so Environment.GetEnvironmentVariable
reads values from .env.test; keep the existing _requiredVars assignment and then
compute missing and set SkipReason as before, and ensure the loader is tolerant
if .env.test is absent (no exception).
In `@PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs`:
- Around line 24-31: The mock currently dequeues the next MockHttpResponse for
any request; update MockHttpMessageHandler and MockHttpResponse so each queued
response includes expected HttpMethod and URL pattern and make the request
handler (SendAsync) validate the incoming HttpRequestMessage against those
expectations: if the head response matches method and URL pattern then dequeue
and return it, otherwise throw an exception signaling an unexpected request
(include actual request method/URL and expected values). Also update the
RespondWith overloads (the methods that enqueue MockHttpResponse) to
accept/record expected method and URL pattern and ensure unit tests using
RespondWith supply those values.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs`:
- Around line 142-143: The registration registers only the concrete
PeachPaymentsGateway, so it won't be discovered when PaymentService resolves
IEnumerable<IPaymentGateway>; change the DI registration(s) from
services.AddScoped<PeachPaymentsGateway>() to
services.AddScoped<IPaymentGateway, PeachPaymentsGateway>() (and apply the same
fix to the other occurrence around the second registration block), ensuring
IPaymentGateway is the service type so PaymentService receives
PeachPaymentsGateway via IEnumerable<IPaymentGateway>.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs`:
- Around line 195-197: The refund payload in PeachPaymentsGateway is hardcoding
["currency"] = "ZAR"; update the refund construction to use the actual
transaction currency (e.g., request.Currency or the original payment's currency)
instead of "ZAR" so refunds honor multi-currency flows; locate the refund
payload assembly in PeachPaymentsGateway (where request.Amount.ToString("F2",
...) is used) and replace the literal "ZAR" with the appropriate currency
property from the request/transaction object and add a null/empty fallback if
needed.
- Around line 143-145: The parsing assumes JsonDocument.Parse(body).RootElement
is an array and directly indexes json[0], which throws on an empty array; update
the parsing in PeachPaymentsGateway (where json and payment are set) to first
check if json.ValueKind == JsonValueKind.Array and json.GetArrayLength() == 0
and in that case treat it as a failed verification (return the normal failed
verification response) instead of accessing json[0]; otherwise proceed to set
payment = json.ValueKind == JsonValueKind.Array ? json[0] : json and continue.
- Around line 206-223: The refund handler currently parses 'body' and inspects
JSON properties (resultEl, codeEl, idEl) without validating the HTTP response
status; update the refund method in PeachPaymentsGateway (the code that builds
the RefundResponse using json, resultEl, resultCode, refundId) to first check
the HTTP response's IsSuccessStatusCode (or StatusCode) and treat non-success
responses as failures: avoid parsing/assuming JSON for error responses, set
Success=false, Status=PaymentStatus.Failed, include a meaningful Message using
the HTTP status and any safe body text, and only parse result/code/description
into RefundResponse when the HTTP status is successful. Ensure
RefundResponse.RefundReference/TransactionReference/Amount remain populated as
appropriate when marking a failure.
In `@PayBridge.SDK/Services/PaymentService.cs`:
- Around line 262-269: The currency routing is incorrectly including
PaymentGatewayType.PeachPayments for XOF, XAF and MWK even though PeachPayments
only supports ZAR/KES/NGN/BWP/USD; update the switch branch that returns
ChooseAvailableGateway for those currencies to remove
PaymentGatewayType.PeachPayments so the call becomes
ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup,
PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); ensure only
gateways declared to support those currencies remain in that
ChooseAvailableGateway invocation.
---
Nitpick comments:
In `@PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs`:
- Around line 77-130: Add two tests: one covering
MockHttpMessageHandler.AssertLastMethod and one covering IntegrationTestBase's
env-var-driven skip logic. For AssertLastMethod, mirror existing request-path
tests: create a MockHttpMessageHandler, RespondWith(...), perform a request with
HttpMethod.Post (and another with HttpMethod.Get), then call
handler.AssertLastMethod(HttpMethod.Post) to assert it does not throw and a
mismatched call to AssertLastMethod expects InvalidOperationException with a
message containing the expected vs actual method. For IntegrationTestBase, add
tests that set and unset the environment variable that IntegrationTestBase reads
(e.g., RUN_INTEGRATION_TESTS or the actual key used in IntegrationTestBase) and
assert the class/method that determines skipping
(IntegrationTestBase.Skip/ShouldSkip/ShouldRunIntegrationTests) returns the
expected boolean for both enabled and disabled values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5426765f-8870-4ee5-8e7f-e80a5eaff852
📒 Files selected for processing (12)
PayBridge.SDK.Test/Helpers/IntegrationTestBase.csPayBridge.SDK.Test/Helpers/MockHttpMessageHandler.csPayBridge.SDK.Test/Helpers/PaymentRequestFactory.csPayBridge.SDK.Test/PayBridge.SDK.Test.csprojPayBridge.SDK.Test/Unit/TestInfrastructureTests.csPayBridge.SDK.Test/paybridge.runsettingsPayBridge.SDK/Dtos/PaymentGatewayConfig.csPayBridge.SDK/Enums/PaymentGatewayType.csPayBridge.SDK/Externsions/IServiceCollectionExtensions.csPayBridge.SDK/Factories/PaymentGatewayFactory.csPayBridge.SDK/Gateways/PeachPaymentsGateway.csPayBridge.SDK/Services/PaymentService.cs
Comment on lines
+36
to
+48
| protected IntegrationTestBase(params string[] requiredEnvVars) | ||
| { | ||
| _requiredVars = requiredEnvVars; | ||
|
|
||
| var missing = requiredEnvVars | ||
| .Where(v => string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(v))) | ||
| .ToList(); | ||
|
|
||
| if (missing.Count > 0) | ||
| { | ||
| SkipReason = $"Integration test skipped — missing env var(s): {string.Join(", ", missing)}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
.env.test source is not loaded by this base class.
This implementation only reads process environment variables; it does not load values from .env.test, which is part of the stated integration-test objective. Please add explicit .env.test loading (or equivalent bootstrap) before missing-var evaluation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs` around lines 36 - 48,
Modify the IntegrationTestBase constructor to load the .env.test file before
evaluating required env vars: call your dotenv loader (e.g.,
DotNetEnv.Load(".env.test") or equivalent safe loader) at the start of the
constructor (before computing missing) so Environment.GetEnvironmentVariable
reads values from .env.test; keep the existing _requiredVars assignment and then
compute missing and set SkipReason as before, and ensure the loader is tolerant
if .env.test is absent (no exception).
Comment on lines
+24
to
+31
| public MockHttpMessageHandler RespondWith( | ||
| HttpStatusCode statusCode, | ||
| string jsonBody, | ||
| string contentType = "application/json") | ||
| { | ||
| _queue.Enqueue(new MockHttpResponse(statusCode, jsonBody, contentType)); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Mock queue does not enforce expected HTTP method/URL per response.
The handler currently dequeues the next response for any incoming request, so a wrong endpoint/method can still pass if the queue is non-empty. That misses the acceptance criterion for queued (method, URL pattern, statusCode, responseBody) matching and “unexpected request” failures.
Also applies to: 33-41, 49-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs` around lines 24 - 31,
The mock currently dequeues the next MockHttpResponse for any request; update
MockHttpMessageHandler and MockHttpResponse so each queued response includes
expected HttpMethod and URL pattern and make the request handler (SendAsync)
validate the incoming HttpRequestMessage against those expectations: if the head
response matches method and URL pattern then dequeue and return it, otherwise
throw an exception signaling an unexpected request (include actual request
method/URL and expected values). Also update the RespondWith overloads (the
methods that enqueue MockHttpResponse) to accept/record expected method and URL
pattern and ensure unit tests using RespondWith supply those values.
Comment on lines
+142
to
143
| services.AddScoped<PeachPaymentsGateway>(); | ||
| return; |
There was a problem hiding this comment.
Register PeachPayments as IPaymentGateway, not only concrete type.
PaymentService builds _gateways from IEnumerable<IPaymentGateway>. Current registration adds only PeachPaymentsGateway, so PeachPayments can be missing from runtime gateway selection.
Suggested fix
- services.AddScoped<PeachPaymentsGateway>();
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
...
- case PaymentGatewayType.PeachPayments:
- services.AddScoped<PeachPaymentsGateway>();
- break;
+ case PaymentGatewayType.PeachPayments:
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
+ break;Also applies to: 194-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs` around lines 142 -
143, The registration registers only the concrete PeachPaymentsGateway, so it
won't be discovered when PaymentService resolves IEnumerable<IPaymentGateway>;
change the DI registration(s) from services.AddScoped<PeachPaymentsGateway>() to
services.AddScoped<IPaymentGateway, PeachPaymentsGateway>() (and apply the same
fix to the other occurrence around the second registration block), ensuring
IPaymentGateway is the service type so PaymentService receives
PeachPaymentsGateway via IEnumerable<IPaymentGateway>.
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
There was a problem hiding this comment.
Guard empty array responses in verification parsing.
When /payments returns an empty array, json[0] throws and the method fails via exception instead of returning a normal failed verification response.
Suggested fix
- var json = JsonDocument.Parse(body).RootElement;
- var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json;
+ var json = JsonDocument.Parse(body).RootElement;
+ JsonElement payment;
+ if (json.ValueKind == JsonValueKind.Array)
+ {
+ if (json.GetArrayLength() == 0)
+ {
+ return new VerificationResponse
+ {
+ Success = false,
+ TransactionReference = transactionReference,
+ Status = PaymentStatus.Failed,
+ Message = "PeachPayments verification returned no payment records"
+ };
+ }
+ payment = json[0];
+ }
+ else
+ {
+ payment = json;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var json = JsonDocument.Parse(body).RootElement; | |
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | |
| var json = JsonDocument.Parse(body).RootElement; | |
| JsonElement payment; | |
| if (json.ValueKind == JsonValueKind.Array) | |
| { | |
| if (json.GetArrayLength() == 0) | |
| { | |
| return new VerificationResponse | |
| { | |
| Success = false, | |
| TransactionReference = transactionReference, | |
| Status = PaymentStatus.Failed, | |
| Message = "PeachPayments verification returned no payment records" | |
| }; | |
| } | |
| payment = json[0]; | |
| } | |
| else | |
| { | |
| payment = json; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 143 - 145, The
parsing assumes JsonDocument.Parse(body).RootElement is an array and directly
indexes json[0], which throws on an empty array; update the parsing in
PeachPaymentsGateway (where json and payment are set) to first check if
json.ValueKind == JsonValueKind.Array and json.GetArrayLength() == 0 and in that
case treat it as a failed verification (return the normal failed verification
response) instead of accessing json[0]; otherwise proceed to set payment =
json.ValueKind == JsonValueKind.Array ? json[0] : json and continue.
Comment on lines
+195
to
+197
| ["amount"] = request.Amount.ToString("F2", System.Globalization.CultureInfo.InvariantCulture), | ||
| ["currency"] = "ZAR", | ||
| ["paymentType"] = "RF", |
There was a problem hiding this comment.
Refund currency is hardcoded to ZAR in a multi-currency gateway flow.
Create payment supports multiple currencies, but refund payload always sends ZAR. Non-ZAR transactions can fail or be refunded against the wrong currency context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 195 - 197, The
refund payload in PeachPaymentsGateway is hardcoding ["currency"] = "ZAR";
update the refund construction to use the actual transaction currency (e.g.,
request.Currency or the original payment's currency) instead of "ZAR" so refunds
honor multi-currency flows; locate the refund payload assembly in
PeachPaymentsGateway (where request.Amount.ToString("F2", ...) is used) and
replace the literal "ZAR" with the appropriate currency property from the
request/transaction object and add a null/empty fallback if needed.
Comment on lines
+206
to
+223
| var json = JsonDocument.Parse(body).RootElement; | ||
| var resultCode = json.TryGetProperty("result", out var resultEl) && | ||
| resultEl.TryGetProperty("code", out var codeEl) | ||
| ? codeEl.GetString() : null; | ||
|
|
||
| bool success = resultCode != null && resultCode.StartsWith("000"); | ||
| var refundId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() ?? string.Empty : string.Empty; | ||
|
|
||
| return new RefundResponse | ||
| { | ||
| Success = success, | ||
| RefundReference = refundId, | ||
| TransactionReference = request.TransactionReference, | ||
| Amount = request.Amount, | ||
| Status = success ? PaymentStatus.Refunded : PaymentStatus.Failed, | ||
| Message = resultEl.TryGetProperty("description", out var descEl) | ||
| ? descEl.GetString() ?? "Unknown" : "Unknown" | ||
| }; |
There was a problem hiding this comment.
Refund flow should validate HTTP status before trusting payload.
The method currently derives refund success from JSON content without checking IsSuccessStatusCode, which can misclassify failed HTTP calls and is fragile for non-JSON error bodies.
Suggested fix
var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content);
var body = await response.Content.ReadAsStringAsync();
_logger.LogDebug("PeachPayments refund response: {Body}", body);
+if (!response.IsSuccessStatusCode)
+{
+ return new RefundResponse
+ {
+ Success = false,
+ TransactionReference = request.TransactionReference,
+ Amount = request.Amount,
+ Status = PaymentStatus.Failed,
+ Message = $"PeachPayments refund failed: {response.StatusCode}"
+ };
+}
+
var json = JsonDocument.Parse(body).RootElement;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 206 - 223, The
refund handler currently parses 'body' and inspects JSON properties (resultEl,
codeEl, idEl) without validating the HTTP response status; update the refund
method in PeachPaymentsGateway (the code that builds the RefundResponse using
json, resultEl, resultCode, refundId) to first check the HTTP response's
IsSuccessStatusCode (or StatusCode) and treat non-success responses as failures:
avoid parsing/assuming JSON for error responses, set Success=false,
Status=PaymentStatus.Failed, include a meaningful Message using the HTTP status
and any safe body text, and only parse result/code/description into
RefundResponse when the HTTP status is successful. Ensure
RefundResponse.RefundReference/TransactionReference/Amount remain populated as
appropriate when marking a failure.
Comment on lines
262
to
269
| case "XOF": | ||
| case "XAF": | ||
| case "MWK": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
|
|
||
| case "BWP": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.DpoGroup); | ||
|
|
There was a problem hiding this comment.
Currency routing includes PeachPayments for currencies outside its declared support.
This selection path now prioritizes PeachPayments for XOF, XAF, and MWK, while the gateway itself documents support for ZAR/KES/NGN/BWP/USD. That mismatch increases avoidable payment failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Services/PaymentService.cs` around lines 262 - 269, The
currency routing is incorrectly including PaymentGatewayType.PeachPayments for
XOF, XAF and MWK even though PeachPayments only supports ZAR/KES/NGN/BWP/USD;
update the switch branch that returns ChooseAvailableGateway for those
currencies to remove PaymentGatewayType.PeachPayments so the call becomes
ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup,
PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); ensure only
gateways declared to support those currencies remain in that
ChooseAvailableGateway invocation.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces shared test infrastructure for the PayBridge.SDK.Test project, but it also adds a new production payment gateway (Peach Payments) and updates runtime gateway-selection logic in PayBridge.SDK.
Changes:
- Adds shared test helpers:
MockHttpMessageHandler, request/config factories, andIntegrationTestBase, plus unit tests and a.runsettingsfile for filtering/coverage. - Introduces
PeachPaymentsGatewayand wires it into DI/factory/config + updates gateway selection and reference-prefix detection. - Updates test project dependencies to include xUnit, FluentAssertions, Moq, and coverage tooling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Adds PeachPayments to currency-based gateway routing and reference-prefix detection. |
| PayBridge.SDK/Gateways/PeachPaymentsGateway.cs | New Peach Payments gateway implementation (create/verify/refund). |
| PayBridge.SDK/Factories/PaymentGatewayFactory.cs | Includes PeachPayments in gateway instantiation. |
| PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs | Registers PeachPaymentsGateway in DI. |
| PayBridge.SDK/Enums/PaymentGatewayType.cs | Adds PeachPayments enum value. |
| PayBridge.SDK/Dtos/PaymentGatewayConfig.cs | Adds PeachPaymentsConfig to gateway configuration. |
| PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs | Adds unit tests for the new test infrastructure (partial coverage). |
| PayBridge.SDK.Test/PayBridge.SDK.Test.csproj | Adds test dependencies and project reference to SDK. |
| PayBridge.SDK.Test/paybridge.runsettings | Adds test filtering guidance and code coverage scoping. |
| PayBridge.SDK.Test/Helpers/PaymentRequestFactory.cs | Adds PaymentRequestFactory and GatewayConfigFactory builders for tests. |
| PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs | Adds queued-response HTTP handler for tests. |
| PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs | Adds base class for env-var-gated integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+265
to
+268
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
|
|
||
| case "BWP": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.DpoGroup); |
Comment on lines
262
to
269
| case "XOF": | ||
| case "XAF": | ||
| case "MWK": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
|
|
||
| case "BWP": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.DpoGroup); | ||
|
|
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
Comment on lines
+49
to
+66
| protected override Task<HttpResponseMessage> SendAsync( | ||
| HttpRequestMessage request, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| _requests.Add(request); | ||
|
|
||
| if (_queue.Count == 0) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"MockHttpMessageHandler: no queued response for {request.Method} {request.RequestUri}. " + | ||
| "Did you forget to call RespondWith()?"); | ||
| } | ||
|
|
||
| var mock = _queue.Dequeue(); | ||
| var response = new HttpResponseMessage(mock.StatusCode) | ||
| { | ||
| Content = new StringContent(mock.Body, Encoding.UTF8, mock.ContentType) | ||
| }; |
Comment on lines
+21
to
+38
| private readonly string[] _requiredVars; | ||
|
|
||
| /// <summary> | ||
| /// The reason string reported by xUnit when a test is skipped. | ||
| /// Set in the constructor if any env var is missing. | ||
| /// </summary> | ||
| protected string? SkipReason { get; } | ||
|
|
||
| /// <summary> | ||
| /// Whether all required env vars are present. | ||
| /// Use this in test bodies to conditionally Skip: | ||
| /// <code>Skip.If(ShouldSkip, SkipReason);</code> | ||
| /// </summary> | ||
| protected bool ShouldSkip => SkipReason != null; | ||
|
|
||
| protected IntegrationTestBase(params string[] requiredEnvVars) | ||
| { | ||
| _requiredVars = requiredEnvVars; |
Comment on lines
+36
to
+48
| protected IntegrationTestBase(params string[] requiredEnvVars) | ||
| { | ||
| _requiredVars = requiredEnvVars; | ||
|
|
||
| var missing = requiredEnvVars | ||
| .Where(v => string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(v))) | ||
| .ToList(); | ||
|
|
||
| if (missing.Count > 0) | ||
| { | ||
| SkipReason = $"Integration test skipped — missing env var(s): {string.Join(", ", missing)}"; | ||
| } | ||
| } |
Comment on lines
+202
to
+206
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | ||
| var body = await response.Content.ReadAsStringAsync(); | ||
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | ||
|
|
||
| var json = JsonDocument.Parse(body).RootElement; |
- ci-tests.yml: runs on every push/PR to master and feature/* branches - Filters to Category=Unit only (no network, no secrets needed) - Publishes TRX results via dorny/test-reporter as PR comment - Uploads coverage.cobertura.xml artifact - integration-tests.yml: manual trigger (workflow_dispatch) + nightly cron - Runs Category=Integration tests with all 15 gateway env vars injected - Uses a protected 'integration' GitHub environment for secret scoping - fail-on-error=false so missing sandbox keys skip rather than fail
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-tests.yml:
- Line 20: Add the checkout step option to disable credential persistence:
update the actions/checkout step that uses "actions/checkout@v4" to include
"with: persist-credentials: false" so the checkout token is not exposed to
downstream steps; ensure you apply the same change to the other workflow(s) that
call actions/checkout@v4 to keep behavior consistent.
- Line 20: The workflow uses mutable action tags (e.g., actions/checkout@v4)
which should be replaced with pinned commit SHAs to remove supply-chain risk;
locate every "uses:" entry referencing actions/checkout@v4,
actions/setup-dotnet@v4, actions/upload-artifact@v4, dorny/test-reporter@v1, and
softprops/action-gh-release@v2 and update each to the corresponding
repository@<commit-sha> (fetch the latest stable commit SHA from the action's
GitHub repo), commit the updated uses: lines, and run/validate the workflow to
ensure syntax and behavior remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eceb1f09-4ee2-4a3d-93c8-6625c1db54c7
📒 Files selected for processing (2)
.github/workflows/ci-tests.yml.github/workflows/integration-tests.yml
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Shared root cause: checkout token persistence not disabled.
Both workflows should set with: persist-credentials: false on actions/checkout to reduce credential exposure in downstream steps.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 19-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-tests.yml at line 20, Add the checkout step option to
disable credential persistence: update the actions/checkout step that uses
"actions/checkout@v4" to include "with: persist-credentials: false" so the
checkout token is not exposed to downstream steps; ensure you apply the same
change to the other workflow(s) that call actions/checkout@v4 to keep behavior
consistent.
Source: Linters/SAST tools
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List workflow files
echo "== Workflows =="
ls -la .github/workflows || true
echo
# Show lines around the referenced area in ci-tests.yml
echo "== ci-tests.yml (around lines 1-80) =="
nl -ba .github/workflows/ci-tests.yml | sed -n '1,120p'
echo
# Find all workflow files and grep for unpinned action uses (tag-only, major-only, etc.)
echo "== Unpinned action references (uses: owner/repo@v*) =="
rg -n "uses:\s*[^/@]+/[^/@]+@v[^#\s]+" .github/workflows || true
echo
echo "== Unpinned action references (uses: owner/repo@<major>.* without sha) =="
rg -n "uses:\s*[^/@]+/[^/@]+@v[0-9]+(?![0-9a-f]{7,40})" .github/workflows || true
echo
echo "== Pinned action references (uses with sha) =="
rg -n "uses:\s*[^/@]+/[^/@]+@[0-9a-f]{7,40}" .github/workflows || true
echo
# If there are multiple workflows, dump their uses blocks for quick inspection
echo "== Dump uses: lines per workflow =="
for f in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -e "$f" ] || continue
echo "--- $f ---"
rg -n "uses:\s*" "$f" || true
doneRepository: teesofttech/PayBridge
Length of output: 495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Workflows =="
ls -la .github/workflows || true
echo
echo "== ci-tests.yml (show full file with line numbers) =="
cat -n .github/workflows/ci-tests.yml | sed -n '1,200p'
echo
echo "== integration-tests.yml (show full file with line numbers) =="
cat -n .github/workflows/integration-tests.yml | sed -n '1,260p'
echo
echo "== nuget-publish.yml (show full file with line numbers) =="
cat -n .github/workflows/nuget-publish.yml | sed -n '1,260p'
echo
echo "== Unpinned action references (uses: owner/repo@v* without sha) across workflows =="
# Matches uses: owner/repo@v<digits/letters> (common tags like v4, v3.1, v2) excluding sha-like 7-40 hex
rg -n --hidden --no-ignore-vcs "uses:\s*[^/@]+/[^/@]+@v[0-9][^#\s]*" .github/workflows || true
echo "== Uses references that look like they include a full/partial commit sha =="
rg -n --hidden --no-ignore-vcs "uses:\s*[^/@]+/[^/@]+@[0-9a-f]{7,40}" .github/workflows || true
echo "== All uses: lines across workflows =="
rg -n --hidden --no-ignore-vcs "^\s*uses:\s*" .github/workflows || trueRepository: teesofttech/PayBridge
Length of output: 11419
Pin GitHub Actions to immutable commit SHAs (avoid tag-based uses:).
CI/integration/release workflows use mutable major tags (no SHA), e.g. actions/checkout@v4, actions/setup-dotnet@v4, actions/upload-artifact@v4, dorny/test-reporter@v1, and softprops/action-gh-release@v2 (see .github/workflows/ci-tests.yml, .github/workflows/integration-tests.yml, .github/workflows/nuget-publish.yml). This creates a supply-chain risk if a tag is moved or a compromised action version is published.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 19-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-tests.yml at line 20, The workflow uses mutable action
tags (e.g., actions/checkout@v4) which should be replaced with pinned commit
SHAs to remove supply-chain risk; locate every "uses:" entry referencing
actions/checkout@v4, actions/setup-dotnet@v4, actions/upload-artifact@v4,
dorny/test-reporter@v1, and softprops/action-gh-release@v2 and update each to
the corresponding repository@<commit-sha> (fetch the latest stable commit SHA
from the action's GitHub repo), commit the updated uses: lines, and run/validate
the workflow to ensure syntax and behavior remain correct.
Source: Linters/SAST tools
- Remove double-dash from XML comments in paybridge.runsettings (invalid XML: 'An XML comment cannot contain --') - Remove --settings flag from both workflows (runsettings only needed for local coverage; was causing the runner to reject it) - Rename workflow title to 'CI - Unit Tests' (dash safe in YAML) - Add fail-on-empty: false to dorny/test-reporter so it does not error when the TRX file is absent due to an upstream failure
GitHub denies test-reporter from creating check runs on PRs unless the workflow explicitly grants 'checks: write' and 'pull-requests: write'. Added permissions block to both ci-tests.yml and integration-tests.yml.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci-tests.yml (1)
14-16: ⚡ Quick winAdd a job timeout to prevent runaway CI jobs.
The job has no
timeout-minutesset. Adding a timeout prevents runaway jobs from consuming CI resources indefinitely and provides faster feedback when something hangs.⏱️ Suggested addition
unit-tests: name: Unit Tests (.NET 8) runs-on: ubuntu-latest + timeout-minutes: 15(Adjust the timeout value based on typical execution time; 15 minutes is a reasonable starting point for unit tests.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-tests.yml around lines 14 - 16, The CI job "unit-tests" currently has no timeout and can hang indefinitely; add a timeout by setting the GitHub Actions job field timeout-minutes under the unit-tests job (e.g., timeout-minutes: 15 or another appropriate value) to limit how long the unit-tests job can run and prevent runaway CI usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci-tests.yml:
- Around line 14-16: The CI job "unit-tests" currently has no timeout and can
hang indefinitely; add a timeout by setting the GitHub Actions job field
timeout-minutes under the unit-tests job (e.g., timeout-minutes: 15 or another
appropriate value) to limit how long the unit-tests job can run and prevent
runaway CI usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64860cd1-ee53-4d99-90a4-5c60fc36d6ad
📒 Files selected for processing (2)
.github/workflows/ci-tests.yml.github/workflows/integration-tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-tests.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #51
Implements all shared test infrastructure that every per-gateway test file will depend on.
What's included
Helpers/MockHttpMessageHandler.csHttpMessageHandler— no live network calls.RespondWith()chainingAssertRequestCount,AssertLastRequestPath,AssertLastMethodBuildClient(baseAddress)convenience methodHelpers/PaymentRequestFactory.csPaymentRequestFactory.Build(configure?)— validPaymentRequestwith all fields setPaymentRequestFactory.BuildRefund(configure?)— validRefundRequestGatewayConfigFactory.Build{GatewayName}(...)— one builder per gateway (all 15)Helpers/IntegrationTestBase.csSkipIfMissingEnvVars()paybridge.runsettingsdotnet test --filter Category=Unit→ unit tests onlydotnet test --filter Category=Integration→ integration tests onlyPayBridge.SDK.dllUnit/TestInfrastructureTests.csTest results
Summary by CodeRabbit
New Features
Tests
Chores